Skip to content

CHANGE: Use slider rather than float field#2317

Closed
josepmariapujol-unity wants to merge 4 commits intodevelopfrom
input/removing-legacy-todos-7
Closed

CHANGE: Use slider rather than float field#2317
josepmariapujol-unity wants to merge 4 commits intodevelopfrom
input/removing-legacy-todos-7

Conversation

@josepmariapujol-unity
Copy link
Collaborator

@josepmariapujol-unity josepmariapujol-unity commented Jan 9, 2026 •
edited
Loading

Description

Use slider rather than float field

Testing status & QA

Nothing yet

Overall Product Risks

Please rate the potential complexity and halo effect from low to high for the reviewers. Note down potential risks to specific Editor branches if any.

  • Complexity: low
  • Halo Effect: low

Comments to reviewers

Please describe any additional information such as what to focus on, or historical info for the reviewers.

Checklist

Before review:

  • Changelog entry added.
    • Explains the change in Changed, Fixed, Added sections.
    • For API change contains an example snippet and/or migration example.
    • JIRA ticket linked, example (case %%). If it is a private issue, just add the case ID without a link.
    • Jira port for the next release set as "Resolved".
  • Tests added/changed, if applicable.
    • Functional tests Area_CanDoX, Area_CanDoX_EvenIfYIsTheCase, Area_WhenIDoX_AndYHappens_ThisIsTheResult.
    • Performance tests.
    • Integration tests.
  • Docs for new/changed API's.
    • Xmldoc cross references are set correctly.
    • Added explanation how the API works.
    • Usage code examples added.
    • The manual is updated, if needed.

During merge:

  • Commit message for squash-merge is prefixed with one of the list:
    • NEW: ___.
    • FIX: ___.
    • DOCS: ___.
    • CHANGE: ___.
    • RELEASE: 1.1.0-preview.3.

@josepmariapujol-unity josepmariapujol-unity self-assigned this Jan 9, 2026
@josepmariapujol-unity josepmariapujol-unity changed the title Input/removing legacy todos 7 CHANGE: Use slider rather than float field Jan 9, 2026
@josepmariapujol-unity josepmariapujol-unity marked this pull request as ready for review January 9, 2026 15:06
@u-pr
Copy link
Contributor

u-pr bot commented Jan 9, 2026

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪

Very small UI-only change (float field to slider) with minimal surface area and straightforward validation needed.
🏅 Score: 88

Clean improvement that removes a TODO, but it hardcodes a 0–1 range which should be validated against the actual parameter semantics and edge cases.
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Range Assumption

The new slider clamps input to 0–1; confirm the underlying parameter is always normalized and that clamping won’t silently change previously valid values (including negatives or >1) for any consumers.

 
const float k_MinValue = 0f;
const float k_MaxValue = 1f;
var newValue = EditorGUILayout.Slider(m_ValueLabel, value, k_MinValue, k_MaxValue, GUILayout.ExpandWidth(false));
if (!m_UseDefaultValue)
    SetValue(newValue);
UX Consistency

Slider precision/step behavior differs from a float field; verify that required precision can still be entered (e.g., via direct text entry on the slider) and that the control behaves well with keyboard input and layout constraints.

 
var newValue = EditorGUILayout.Slider(m_ValueLabel, value, k_MinValue, k_MaxValue, GUILayout.ExpandWidth(false));
  • Update review

🤖 Helpful? Please react with 👍/👎 | Questions❓Please reach out in Slack #ask-u-pr-agent

@u-pr
Copy link
Contributor

u-pr bot commented Jan 9, 2026

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Clamp values to slider range

Clamp value before drawing the slider to avoid Unity UI warnings/odd jumps when the
current value is outside [0,1] (e.g., loaded from older data). Also clamp newValue
before persisting to guarantee the stored value stays within the supported range.

Packages/com.unity.inputsystem/InputSystem/Editor/InputParameterEditor.cs [364-369]

 const float k_MinValue = 0f;
 const float k_MaxValue = 1f;
+
+value = Mathf.Clamp(value, k_MinValue, k_MaxValue);
 var newValue = EditorGUILayout.Slider(m_ValueLabel, value, k_MinValue, k_MaxValue, GUILayout.ExpandWidth(false));
+
 if (!m_UseDefaultValue)
-    SetValue(newValue);
+    SetValue(Mathf.Clamp(newValue, k_MinValue, k_MaxValue));
Suggestion importance[1-10]: 6

__

Why: Clamping value (and the persisted newValue) to [0,1] avoids potential IMGUI slider weirdness if serialized/legacy data contains out-of-range values, and makes SetValue consistent with the UI constraint. It’s a reasonable robustness improvement, though the slider already constrains user input so the main benefit is handling preexisting out-of-range values.

Low
  • More suggestions

🤖 Helpful? Please react with 👍/👎 | Questions❓Please reach out in Slack #ask-u-pr-agent

@codecov-github-com
Copy link

codecov-github-com bot commented Jan 9, 2026 •
edited
Loading

Codecov Report

Attention: Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
...tsystem/InputSystem/Editor/InputParameterEditor.cs 0.00% 1 Missing ⚠️ 
@@           Coverage Diff            @@
##           develop    #2317   +/-   ##
========================================
  Coverage    77.95%   77.95%           
========================================
  Files          476      476           
  Lines        97443    97453   +10     
========================================
+ Hits         75961    75971   +10     
  Misses       21482    21482
Flag Coverage Δ
inputsystem_MacOS_2022.3 5.54% <0.00%> (ø)
inputsystem_MacOS_2022.3_project 75.47% <0.00%> (ø)
inputsystem_MacOS_6000.0 5.32% <0.00%> (ø)
inputsystem_MacOS_6000.0_project 77.36% <0.00%> (ø)
inputsystem_MacOS_6000.3 5.32% <0.00%> (ø)
inputsystem_MacOS_6000.3_project 77.36% <0.00%> (ø)
inputsystem_MacOS_6000.4 5.32% <0.00%> (ø)
inputsystem_MacOS_6000.5 5.32% <0.00%> (ø)
inputsystem_MacOS_6000.5_project 77.36% <0.00%> (ø)
inputsystem_Ubuntu_2022.3 5.54% <0.00%> (ø)
inputsystem_Ubuntu_2022.3_project 75.26% <0.00%> (ø)
inputsystem_Ubuntu_6000.0 5.32% <0.00%> (ø)
inputsystem_Ubuntu_6000.0_project 77.16% <0.00%> (-0.01%) ⬇️
inputsystem_Ubuntu_6000.3 5.32% <0.00%> (ø)
inputsystem_Ubuntu_6000.3_project 77.16% <0.00%> (-0.01%) ⬇️
inputsystem_Ubuntu_6000.4 5.33% <0.00%> (ø)
inputsystem_Ubuntu_6000.4_project 77.17% <0.00%> (-0.01%) ⬇️
inputsystem_Ubuntu_6000.5 5.33% <0.00%> (ø)
inputsystem_Ubuntu_6000.5_project 77.18% <0.00%> (+<0.01%) ⬆️
inputsystem_Windows_2022.3 5.54% <0.00%> (ø)
inputsystem_Windows_2022.3_project 75.60% <0.00%> (+<0.01%) ⬆️
inputsystem_Windows_6000.0 5.32% <0.00%> (ø)
inputsystem_Windows_6000.0_project 77.49% <0.00%> (+<0.01%) ⬆️
inputsystem_Windows_6000.3 5.32% <0.00%> (ø)
inputsystem_Windows_6000.3_project 77.49% <0.00%> (ø)
inputsystem_Windows_6000.4 5.32% <0.00%> (ø)
inputsystem_Windows_6000.4_project 77.49% <0.00%> (-0.01%) ⬇️
inputsystem_Windows_6000.5 5.32% <0.00%> (ø)
inputsystem_Windows_6000.5_project 77.49% <0.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...tsystem/InputSystem/Editor/InputParameterEditor.cs 16.95% <0.00%> (ø)

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

josepmariapujol-unity
josepmariapujol-unity commented Jan 13, 2026
View reviewed changes
Packages/com.unity.inputsystem/InputSystem/Editor/InputParameterEditor.cs
////TODO: use slider rather than float field
var newValue = EditorGUILayout.FloatField(m_ValueLabel, value, GUILayout.ExpandWidth(false));
const float k_MinValue = 0f;
const float k_MaxValue = 1f;
Copy link
Collaborator Author

@josepmariapujol-unity josepmariapujol-unity Jan 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should 1f be the max value?

@josepmariapujol-unity josepmariapujol-unity marked this pull request as draft January 15, 2026 12:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@josepmariapujol-unity josepmariapujol-unity

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@josepmariapujol-unity